Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests: harden TestNetwork and TestNmt even more #508

Merged

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jul 9, 2024

  • test task.stop()
  • use more lenient timeout
  • make sure we always collect two frames

See commit messages for details.

@erlend-aasland
Copy link
Contributor Author

Follow-up for shortcomings in #505.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jul 9, 2024

@acolomb, some rationale for the changes:

  • During tests with macOS runners, I found that they very often timed out1, hence I've increased the timeout considerably.
  • Since .update() restarts the timer, we can't use a frame from dataset 1 and a frame from dataset 2 to validate the periodicity; we need to collect exactly two frames from each "session" in order to test this deterministically.
  • Wait for periodicity to be established; some platforms/runners are observed to be flaky
  • Added missing test of task.stop() functionality.
  • Introduced a check() helper for the sake of readability.
  • Rewritten to use threading.Condition for the sake of readability.

I think this should be fine. I'll mark it as ready for review tomorrow morning CET ☕

Footnotes

  1. I've only seen this on macOS runners, never on Ubuntu nor Windows.

- take into account that payloads with DATA1 may be in flight when we
  update the task
- test task.stop()
- use more lenient timeout
- use threading.Condition for improved readability
- wait for periodicity to be established before validation; some
  platforms are a little bit flaky
@erlend-aasland erlend-aasland marked this pull request as ready for review July 10, 2024 09:43
@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.86%. Comparing base (a196e1e) to head (0363ece).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #508   +/-   ##
=======================================
  Coverage   68.86%   68.86%           
=======================================
  Files          26       26           
  Lines        3112     3112           
  Branches      526      526           
=======================================
  Hits         2143     2143           
  Misses        831      831           
  Partials      138      138           

test/test_network.py Outdated Show resolved Hide resolved
@erlend-aasland erlend-aasland changed the title Tests: harden TestNetwork even more Tests: harden TestNetwork and TestNmt even more Jul 10, 2024
test/test_network.py Outdated Show resolved Hide resolved
test/test_nmt.py Outdated Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor Author

After this PR has landed, I'll follow-up with a small PR to expand the CI matrix so we can run the test suite on macOS and Windows, as well as on Linux. That should prevent failures like #501.

@sveinse
Copy link
Collaborator

sveinse commented Jul 10, 2024

Do we know what account @christiansandberg have? Are we on a free tier or something paid for? And are we well away from any spending limits on actions? -- Adding new CI matrixes is great, but it triples build minutes, so I think maybe we should give the owner a chance to ok this.

@erlend-aasland
Copy link
Contributor Author

Are we on a free tier or something paid for? And are we well away from any spending limits on actions? -- Adding new CI matrixes is great, but it triples build minutes, so I think maybe we should give the owner a chance to ok this.

Ordinary user accounts with public repos are given a pretty large batch of CI minutes per month for free. Anyway, let's discuss this in the following-up PR; it's not related to this change.

@acolomb acolomb merged commit ecf216a into christiansandberg:master Aug 6, 2024
3 checks passed
@erlend-aasland erlend-aasland deleted the test/revisit-send-periodic branch August 6, 2024 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants